Skip to content

fix(cudf): Handle sliced decimal aggregation state - #18608

Open
mattgara wants to merge 1 commit into
facebookincubator:mainfrom
mattgara:fix-cudf-sliced-decimal-aggregation-state
Open

fix(cudf): Handle sliced decimal aggregation state#18608
mattgara wants to merge 1 commit into
facebookincubator:mainfrom
mattgara:fix-cudf-sliced-decimal-aggregation-state

Conversation

@mattgara

@mattgara mattgara commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator
  • Respect sliced string-column offsets when validating and unpacking decimal aggregation state. This keeps payload bounds, string offsets, and validity mask indices aligned with the sliced view.

  • Add a regression test for deserializing a sliced decimal aggregation state with a non-zero parent offset and a null row.

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 20, 2026
@netlify

netlify Bot commented Aug 20, 2026

Copy link
Copy Markdown

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 3c90df7
🔍 Latest deploy log https://app.netlify.com/projects/meta-velox/deploys/6a8778488cf5570008cc8a98

@github-actions github-actions Bot added the cudf cudf related - GPU acceleration label Aug 20, 2026
@github-actions

Copy link
Copy Markdown

CI Failure Analysis

Auto-generated by the CI Failure Analysis workflow. This comment is updated in place each time CI fails on a new commit, so it always reflects the latest run — re-pushing or re-running CI will refresh the analysis below. Last updated 2026-08-20 22:18:42 UTC from workflow run 32421974712.

🔴 Expression Fuzzer with Presto SOT — FUZZER Failure View logs

Fuzzer: Presto Expression Fuzzer with Presto as Source of Truth
Failed instance: 2 of 4 (seed=755394232)

Error: Velox and Presto reference DB results don't match. The fuzzer detected a result mismatch — 1 extra row and 1 missing row out of 100 total rows. The differing value is in a BIGINT field: Velox produced 6222293085463513099 while Presto returned 6222293100209113099.

/__w/velox/velox/velox/velox/exec/tests/utils/QueryAssertions.cpp:1152: Failure
Failed
Expected 100, got 100
1 extra rows, 1 missing rows

Expression: exec::test::assertEqualResults(
    referenceEvalResult.value(),
    projectionPlan->outputType(),
    {commonEvalResultRow})
Reason: Velox and reference DB results don't match

File: velox/expression/tests/ExpressionVerifier.cpp
Line: 480

The fuzzer aborted (core dumped) after the assertion failure.


Correlation with PR changes:

  • Not related. This PR (fix(cudf): Handle sliced decimal aggregation state #18608) only modifies files under velox/experimental/cudf/ — specifically CUDA/GPU decimal aggregation code (DecimalAggregationDevice.cu, DecimalAggregationDevice.h, DecimalAggregationState.cpp, and DecimalAggregationTest.cpp). The fuzzer failure is in the CPU-based Presto expression fuzzer comparing Velox expression evaluation against the Presto reference server. These are entirely separate code paths.

Known issues:

  • There are existing open issues for Presto expression fuzzer result mismatches: #14335 and #14308.
  • The Expression Fuzzer with Presto SOT job passed on the last 8 consecutive main branch runs, so this is not a consistently failing test. However, expression fuzzers are inherently non-deterministic (different seeds each run), so a seed-specific mismatch can surface intermittently.

Reproduce locally:

./velox_expression_fuzzer_test \
    --seed 755394232 \
    --enable_variadic_signatures \
    --velox_fuzzer_enable_complex_types \
    --lazy_vector_generation_ratio 0.2 \
    --common_dictionary_wraps_generation_ratio=0.3 \
    --velox_fuzzer_enable_column_reuse \
    --velox_fuzzer_enable_expression_reuse \
    --enable_dereference \
    --duration_sec 300 \
    --minloglevel=0 \
    --special_forms="cast,coalesce,if" \
    --velox_fuzzer_max_level_of_nesting=1 \
    --presto_url=http://127.0.0.1:8080

Note: Requires a running Presto server for SOT comparison.

Recommended fix:

@github-actions

Copy link
Copy Markdown

Selective Build Plan

Linux release with adapters is running a full build (changes touch velox/experimental/ or velox/external/). See the CI workflows README for what this means.


Selective build plan

Comment on lines 109 to 111
assert(
offsets[idx + 1] - offsets[idx] ==
offsets[inputIdx + 1] - offsets[inputIdx] ==
static_cast<OffsetT>(detail::kDecimalSumStateSize));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Broader question: Does this assert actually propagate error to host?

requires OffsetStorageType<OffsetT>
void operator()() const {
auto const n = static_cast<size_t>(numRows);
auto const inputSize = static_cast<size_t>(rowOffset) + n;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Should we add a defensive check here to verify that inputSize < offsetsView.size() to guard against malformed inputs?

EXPECT_EQ(outSum[2], static_cast<__int128_t>(40));
EXPECT_EQ(outCount[2], 4);
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Can we also add a test/modify the existing test so it it accepts a sliced view of an arrow compacted column? We can then verify if the parent offsets are correctly applied to both the offsets child and the null mask.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. cudf cudf related - GPU acceleration

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants